Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage fees: storage limiter #80

Merged

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Oct 19, 2020

FLIP reference: onflow/flow#99
task reference: https://github.com/dapperlabs/flow-go/issues/4935

add storage limit checking to the transaction pipeline

@janezpodhostnik janezpodhostnik marked this pull request as ready for review November 2, 2020 19:21
…w/flow-go into janez/storage-fees-storage-limiter
…w/flow-go into janez/storage-fees-storage-limiter
Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking solid! Nice work 👍

fvm/errors.go Outdated Show resolved Hide resolved
fvm/errors.go Outdated Show resolved Hide resolved
fvm/errors.go Outdated Show resolved Hide resolved
fvm/fvm_test.go Outdated Show resolved Hide resolved
fvm/transactionStorageLimiter.go Outdated Show resolved Hide resolved

type TransactionStorageLimiter struct{}

func NewTransactionStorageLimiter() *TransactionStorageLimiter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 👏

fvm/state/ledger.go Outdated Show resolved Hide resolved
}
}

func newMockLedger(updatedKeys []string, ownerKeyStorageValue []OwnerKeyValue) MockLedger {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to use MapLedger in place of this mock? I think you could keep this helper function to seed the ledger state -- I'm just not sure you need to define an extra data structure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to check how many times I access the ledger get function. I don't think I can do that part with the MapLedger. I could remove those tests, since they are only testing that storage capacity checking is done once per account.

Base automatically changed from janez/storage-fees-storage-updates to feature/storage-fees November 17, 2020 15:57
@janezpodhostnik janezpodhostnik merged commit 25135b2 into feature/storage-fees Nov 17, 2020
@janezpodhostnik janezpodhostnik deleted the janez/storage-fees-storage-limiter branch November 17, 2020 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants